GODRIVER-3173 Complete pending reads on conn checkout #1977
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mongodb-drivers-pr-bot
bot
added
the
review-priority-low
API Change Report./v2/eventcompatible changesConnectionPendingResponseFailed: added ./v2/x/mongo/drivercompatible changesRetryablePendingResponseError: added ./v2/x/mongo/driver/topologyincompatible changesBGReadCallback: removed compatible changesPendingResponseTimeout: added |
prestonvasquez
force-pushed
the
GODRIVER-3173
branch
from
aa4debe to
a805a43
Compare
prestonvasquez
changed the title
There was a problem hiding this comment.
Pull Request Overview
This pull request implements changes to support completing pending reads on connection checkout. Key changes include:
- Adding new YAML tests for client‐side operation timeouts related to pending reads.
- Updating connection and pool logic to handle pending reads with context values (e.g. maxTimeMS and requestID) and synchronizing background reads.
- Introducing new monitoring events and updating event verification and test instrumentation to track pending read events.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testdata/client-side-operations-timeout/pending-reads.yml | New YAML tests for handling operation timeouts and pending read scenarios. |
| internal/integration/client_test.go | Updated concurrent operation tests to account for pending reads. |
| internal/driverutil/context.go | Added context helper functions for maxTimeMS and requestID propagation. |
| x/mongo/driver/topology/pool.go | Updated pool checkout logic to await completion of pending reads and renamed timeout constants. |
| x/mongo/driver/topology/pool_test.go | Revised pool tests to verify new pending read behaviors. |
| internal/integration/unified/event_verification.go | Added fields and tests for verifying new pending read pool events. |
| event/monitoring.go | Extended pool event monitoring with pending read event support. |
| x/mongo/driver/operation.go | Propagated context values in operation execution for pending read handling. |
| internal/integration/csot_test.go | Updated CSOT tests to ensure connection closure conditions with pending reads. |
Comments suppressed due to low confidence (2)
internal/integration/unified/event_verification.go:61
- Field name 'ConnectionPendingreadSucceeded' should be 'ConnectionPendingReadSucceeded' to maintain consistent capitalization with the other pending read event fields.
ConnectionPendingreadSucceeded *struct{} `bson:"connectionPendingReadSucceeded"`
x/mongo/driver/topology/pool.go:908
- Typo in comment: 'alawys' should be corrected to 'always'. Consider removing or clarifying this inline question to avoid confusion.
if size == 0 { // Question: Would this alawys equal to zero?
4 tasks
prestonvasquez
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements pending response handling by moving the pending read logic into the foreground and replacing the legacy BGReadCallback mechanism. Key changes include renaming the test function from TestBackgroundRead to TestAwaitPendingRead, replacing the awaitRemainingBytes field with a new pendingResponseState (with corresponding context values), and updating pool and connection logic to support pending response events.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| x/mongo/driver/topology/pool_test.go | Renamed test functions and updated assertions to use pendingResponseState instead of awaitRemainingBytes |
| x/mongo/driver/topology/pool.go | Replaced BGReadTimeout with PendingResponseTimeout and integrated awaitPendingResponse logic with new event publications |
| x/mongo/driver/topology/connection.go | Replaced awaitRemainingBytes with pendingResponseState and updated read methods accordingly |
| Other test and integration files | Adjusted test expectations and event verifications for pending read events |
Comments suppressed due to low confidence (1)
x/mongo/driver/topology/connection.go:929
- [nitpick] Consider removing or clarifying the inline comment about whether size would always equal zero to avoid confusion in future maintenance.
if size == 0 { // Question: Would this alawys equal to zero?
x/mongo/driver/topology/pool_test.go
Outdated
| _, err = conn.readWireMessage(ctx) | ||
| regex := regexp.MustCompile( | ||
| `^connection\(.*\[-\d+\]\) incomplete read of message header: context deadline exceeded: read tcp 127.0.0.1:.*->127.0.0.1:.*: i\/o timeout$`, | ||
| ) | ||
| assert.True(t, regex.MatchString(err.Error()), "error %q does not match pattern %q", err, regex) | ||
| assert.Nil(t, conn.awaitRemainingBytes, "conn.awaitRemainingBytes should be nil") | ||
| close(errsCh) // this line causes a double close if BGReadCallback is ever called. | ||
| assert.Nil(t, conn.pendingResponseState, "conn.awaitRemainingBytes should be nil") |
There was a problem hiding this comment.
Please update the error message to refer to 'conn.pendingResponseState' instead of 'conn.awaitRemainingBytes' for clarity and consistency with the new field name.
Suggested change
| assert.Nil(t, conn.pendingResponseState, "conn.awaitRemainingBytes should be nil") | |
| assert.Nil(t, conn.pendingResponseState, "conn.pendingResponseState should be nil") |
Copilot uses AI. Check for mistakes.
qingyang-hu
reviewed
May 20, 2025
internal/integration/client_test.go
Outdated
| // them to all return a timeout error because the failpoint | ||
| // blocks find operations for 500ms. Run 50 to increase the | ||
| // blocks find operations for 50ms. Run 50 to increase the |
There was a problem hiding this comment.
It has been changed to 150ms.
github-actions
bot
added
the
review-priority-normal
🧪 Performance ResultsCommit SHA: 7ed2567The following benchmark tests for version 68b8792e15924f0007e0a657 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
GODRIVER-3173
Summary
Move pending response logic into the foreground.
To test locally you will have to check out the branch here for testdata/specifications: mongodb/specifications#1675
Background & Motivation
From the specifications update: